Skip to content

[WIP] Android support #310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

colemancda
Copy link
Contributor

WIP branch with changes that enable use of JavaKit on Android.

colemancda and others added 6 commits July 13, 2025 04:03
This is not a real fix, I suspect the real fix is to remove javaEnvironment as
an instance variable on all platforms and always use the ambient environment
(as the JNI specification clearly states the environment cannot be shared
between threads).

Works around: swiftlang#157
@colemancda colemancda marked this pull request as draft July 13, 2025 08:50
@ktoso ktoso added the android label Jul 13, 2025
@ktoso
Copy link
Collaborator

ktoso commented Jul 14, 2025

Do we intend this PR to supersede #144 @colemancda @lhoward ?

@lhoward
Copy link
Contributor

lhoward commented Jul 14, 2025

I haven’t been following this one. Is it a superset of mine? If so, makes sense. If it’s an intersecting set then, I would propose reviewing mine and then rebasing this one after it is merged (presuming it is approved).

@ktoso
Copy link
Collaborator

ktoso commented Jul 14, 2025

I'd like for us to catch up, it's a bit tricky for me to accept PRs without Android CI and without clear picture which ones we should be moving forward with and what is only PoC etc. Both are Draft so I understand them not really intended as being merged, but I also don't want to waste either of your efforts.

Will both of you in the android/java call we're scheduling with the android workgroup?

@lhoward
Copy link
Contributor

lhoward commented Jul 14, 2025

I’ll be. Sorry I only get to check in on this every month or so after an initial spurt of activity!

@ktoso
Copy link
Collaborator

ktoso commented Jul 14, 2025

No problem at all :-) Just trying to figure out what everyone's plans here are 👍

@lhoward
Copy link
Contributor

lhoward commented Jul 14, 2025

It does that appear @colemancda has duplicated much of the work in the original PR, which I suppose is my fault for not following up on trying to get it merged earlier.

Comments on specific commits are below (I've avoided using commit hashes are they're liable to change).

  • [JavaRuntime] Add Android shims: dlopen()'ing libnativehelper.so was in my original patch set, but was removed IIRC at @ktoso's suggestion. NDK versions 31 and above export the necessary symbols. I don't have a particularly strong position on this (if it works, it works), but arguably we shouldn't be using symbols that we can't link at build time.
  • JavaKit: always use ambient javaEnvironment on Android: I approve this fix, as it's cherry-picked from my PR.
  • [JavaKit] Add JNIEnvPointer for Android: this was included in my commit JavaKit: support for Android NDK JVM; no objection to it being a distinct commit, but suggest a more verbose commit message.
  • [JavaKit] Fix JNINativeInterface_ for Android: as above.
  • [JavaKit] Don't include JVM path for Android: as above.

The commit:

  • Samples: add sample Android app

from my PR may be useful to include.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants